Conversation
Why these changes are being introduced: The OpenSearch model has become unwieldy, and will continue to grow as we add semantic search functionality. Relevant ticket(s): - [USE-372](https://mitlibraries.atlassian.net/browse/USE-372) How this addresses that need: This breaks out the OpenSearch logic into multiple builder models: Filter Builder and Lexical Query Builder (with Semantic Query Builder to come later). This refactor is intended to improve maintainability. Side effects of this change: - The OpenSearch model still retains some query building logic, most notably search highlights. We could consider creating a very small HighlightBuilder service if we want to commit to this architectural pattern. - As discussed in the ADR for this change, we will not be implementing the 'Strategy' selector model at this time. That change will come later, once semantic search has been implemented.
JPrevost
left a comment
JPrevost
left a comment
There was a problem hiding this comment.
This works and is a huge improvement. I'm commenting instead of approving in case you want to address any of the non-blocking comments I made. If you prefer as-is just let me know and I'm happy to approve.
app/models/opensearch.rb
Outdated
There was a problem hiding this comment.
Non-blocking.
While we are refactoring, I think it would be nice to move calculate_size to its own method. I'm okay with it staying in the opensearch class, but pulling this to a method would help make build_query easier to read.
app/models/opensearch.rb
Outdated
| @query_strategy.build(@params, @fulltext) | ||
| end | ||
|
|
||
| def sort |
There was a problem hiding this comment.
Non-blocking.
The ADR notes extracting sort to its own Builder even though it is very simple at this time. If it was an intentional decision to leave it here, I'm totally fine with seeing how this feels before changing it, but if it was just overlooked I do slightly prefer extraction.
There was a problem hiding this comment.
Not intentional at all. I can extract it before we merge.
app/models/opensearch.rb
Outdated
There was a problem hiding this comment.
We probably want to think about whether to extract source and highlight to follow the same patterns for the entire query building. They are slightly different as they are conditionally included in the hash. I'm okay with extracting or waiting to see how this class feels after this work lands to see if it warrants change.
There was a problem hiding this comment.
Yeah, I considered a HighlightBuilder class, and source is a good candidate, too. Given that we're extracting source, it makes sense to do all three as part of this change.
- Move highlights, sorting, and source to separate models - Refactor size calculation into method
3 participants
Why these changes are being introduced:
The OpenSearch model has become unwieldy, and will continue to grow as we add semantic search
functionality.
Relevant ticket(s):
How this addresses that need:
This breaks out the OpenSearch logic into multiple builder models: Filter Builder and Lexical Query
Builder (with Semantic Query Builder to come
later). This refactor is intended to improve
maintainability.
Side effects of this change:
We could consider creating a very small
HighlightBuilder service if we want to commit to
this architectural pattern.
at this time. That change will come later, once
semantic search has been implemented.
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO